-
Notifications
You must be signed in to change notification settings - Fork 185
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Several small fixes and enhancements #115
base: master
Are you sure you want to change the base?
Conversation
The approach of connecting xhprof via Composer and then setting a constant (or env variable) indicating actual XHProf config doesn't look too intuitive. Also if installed via Composer you won't be able to view XHProf reports, because I'd like bugfixes part of this PR though, but please remove all indentation changes that makes this PR look too big (lots of changes). |
Hi, I'll try to remove the indentation changes. But about the vendor/ thing: The changes don't require anyone to put xhprof in vendor or outside of the document root. The changes are designed to be optional and completely backwards compatible and not break the current behavior. Same goes for the constant / env variable: The old behavior works as is, no one is forced to do it, but for those who want to keep the configuration out of the vendor/ directory (like in many modern php projects), this gives them this possibility. I'll see what I can to do split the PR. |
I understand that. I just don't see any reason for any project to connect profiling library as it's development dependency so that later same project (xhprof) needs to be cloned elsewhere to access same database, where profiling info was stored into.
Good. |
Sorry if I insist ;-). I think it does: Of course only in the require-dev section of composer.json, and not in the production requirements (require) section. And your point is also exactly the reason behind moving the config.php in a place where configs are located within the project (such as the parameters.yml in Symfony: It is individual per environment, and usually not committed into any repo): to make sure it's something used only in development / profiling. I'm stitching a new commit together with the indents corrected.. |
fd7bbcf
to
4f5eaf1
Compare
So, I tried to remove all indent problems and it seems that I found. The documents already had mixed indents (partially tabs, partially spaces), that's where the confusion came from. I edited index.php and header.php to reflect the original indenting. |
xhprof_lib/utils/xhprof_runs.php
Outdated
global $_xhprof; | ||
if ($_xhprof['display'] === true) | ||
{ | ||
echo "Failed to insert: <pre>$query</pre> <br>\n"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the only changed line if I view diff using https://github.com/preinheimer/xhprof/pull/115/files?w=1 . I guess it's your IDE that making all these indentation changes.
In git you only stage specific lines if you can't from unwanted changes in IDE itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, sorry, again it was due to PHPStorm settings (as you guessed), combined with the fact that many files here have a mixed indentation. I'll fix that right away...
@@ -29,7 +29,7 @@ | |||
|
|||
<tr> | |||
<td style="background-color: <?php echo $color; ?>;"><span style="display: none"><?php echo $color; ?></span> </td> | |||
<td><a href="index.php?run=<?php echo $run1; ?>&symbol=<?php echo urlencode($element['fn']); ?>"><?php echo htmlentities($element['fn'], ENT_QUOTES, 'UTF-8'); ?></a></td> | |||
<td><a href="<?php echo $_xhprof['url']; ?>/index.php?run=<?php echo $run1; ?>&symbol=<?php echo urlencode($element['fn']); ?>"><?php echo htmlentities($element['fn'], ENT_QUOTES, 'UTF-8'); ?></a></td> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In which scenarios without this change something breaks? I have xhprof
installed in a sub-folder of DocumentRoot and all works perfectly fine already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly, this is one of the situations where not having the whole xhprof code in the document root breaks it (but, for example, only Alias as suggested in the original version, or a symlink of xhprof_html). Putting in this not only fixes xhprof for that case, but potentially also enables the use of a dedicated virtual host for it (although I haven't tested the latter yet), which might be very useful when comparing, for example, the results of different development hosts.
xhprof_lib/display/xhprof.php
Outdated
@@ -42,8 +42,9 @@ | |||
* Our coding convention disallows relative paths in hrefs. | |||
* Get the base URL path from the SCRIPT_NAME. | |||
*/ | |||
$base_path = rtrim(dirname($_SERVER['SCRIPT_NAME']), "/"); | |||
|
|||
// $base_path = rtrim(dirname($_SERVER['SCRIPT_NAME']), "/"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to keep old code in commented format. Just replace it. There will be an explanation of made changes in the commit message itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, I seem to have forgotten this along the way. Good catch. Removing it..
@@ -8,10 +8,11 @@ | |||
$_xhprof['dbpass'] = 'password'; | |||
$_xhprof['dbname'] = 'xhprof'; | |||
$_xhprof['dbadapter'] = 'Pdo'; | |||
$_xhprof['servername'] = 'myserver'; | |||
$_xhprof['servername'] = 's01'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why change this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is because the table structure has a CHAR(3) in the current version, and when I first copied these settings, they ended up breaking all INSERTs because "myserver" is 8 chars long. I thought it would be more intuitive to have an example with 3 chars, so no need to change table structure, but it doesn't fail on the first try.
There's another PR in the pipeline that I saw, that would rather change the table structure, but I thought that this change is less "destructive" for current users...
$_xhprof['namespace'] = 'myapp'; | ||
$_xhprof['url'] = 'http://url/to/xhprof/xhprof_html'; | ||
$_xhprof['getparam'] = "_profile"; | ||
$_xhprof['displayparam'] = "_display"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Is this a new parameter or an old one, that wasn't mentioned.
- Would it work correctly for people with config without this parameter?
- If this is a new parameter, then why it's needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- A new one, but actually referring to an already present functionality ($_xhprof['display'])
- Yes, as with "getparam", a default is set when it is not found (see header.php, line 79).
- I thought it would be a nice thing to be able to switch the "display" functionality (which mainly displays a link to the xhprof page for the current run) on/off at runtime, via a parameter - exactly like for the _profile toggle. I like to have the link, but some pages just won't work with it, paricularly JSON output or templates generated for frontend frameworks.
I added the parameter to be "consistent" with the current way of handling the other runtime functionality, which is the "getparam"/profile.
README.markdown
Outdated
@@ -53,22 +53,36 @@ Installation | |||
|
|||
* Install your favourite mix of PHP and web server | |||
* Install MySQL server | |||
* Clone the project to some folder | |||
* Clone the project to some folder of your choice. | |||
* Alternatively, for composer fans: add a repository m in the repositories section of your composer.json: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- what's with
m
letter (looks like a typo) - the
composer.json
should be backticks andcomposer
word beComposer
README.markdown
Outdated
* Clone the project to some folder | ||
* Clone the project to some folder of your choice. | ||
* Alternatively, for composer fans: add a repository m in the repositories section of your composer.json: | ||
<pre> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use Markdown syntax instead of HTML (3 backticks for both opening/closing PRE).
README.markdown
Outdated
<pre> | ||
{ | ||
"type": "git", | ||
"url": "https://github.com/path-to-this-repo.git" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All this large fragment won't be needed if we publish it on Packagist. Please rewrite.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool. Are you going to do the publishing, or shall I take this on? Adding to my change list..
README.markdown
Outdated
"url": "https://github.com/path-to-this-repo.git" | ||
} | ||
</pre> | ||
And then add the requirement `"xhprof/xhgui": "dev-master"` to your composer.json, then `composer update`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can replace this and previous 2 lines with just
Run the "composer require packagename --dev".
* Update the URL of the service (should point to `xhprof_html` over HTTP) | ||
* Update the `dot_binary` configuration - otherwise no call graphs! | ||
* Update the `controlIPs` variable to enable access. | ||
* Update the SQL server configuration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why these lines are indented differently now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it looks more intuitive to me to put all the steps pertaining to editing the configuration under it...
…root and the base path is wrong
…ng, add a _display parameter to toggle displaying of profiler link
…a CHAR(3), so using the old example broke the queries
Hi aik099, I just pushed the latest proposal, which should reflect most if not all of your requests. Please see my comments, and let's agree on a suitable composer.json name for this. preinheimer/xhprof would be perfectly fine for me, but this needs to be in sync with what will be submitted to packagist. Best, Lorenzo |
Hi there,
I just wanted to share a few fixes / enhancements I've been doing while working with xhprof. I know it's a pull request with several changes, but they all boil down to:
If you deem them useful, I'd be happy to refer to the upstream repo instead of my fork in the future.
If there's anything you'd like me to change and rebase, let me know.
Best wishes,
Lorenzo